docs: update resource URI and add plugin support#1468
Conversation
Our resource URI doc is out-of-date. Let's update it to reflect how we actually use resource URIs and to clarify the relationship between resource URIs and the KBS API. This has confused a number of people in the past. Also, let's introduce a mechanism for specifying the plugin in the resource URI. Specifically, let's allow people to specify the plugin in the scheme. Signed-off-by: Tobin Feldman-Fitzthum <tfeldmanfitz@nvidia.com>
| ### Repository, Type, and Tag | ||
|
|
||
| Currently resource URIs have three levels of identifiers/scope. | ||
| The terms `repository`, `type`, and `tag` are somewhat arbitrary. | ||
| These identifiers can be used in any way. |
There was a problem hiding this comment.
It's resource plugin-specific and no requirements on other plugins
There was a problem hiding this comment.
yeah, I was also thinking whether it is a good idea to require the same path segment for every plugin, for some this hierarchy might just not make any sense. but it keeps implementation complexity at bay for now and that requirement can be relaxed later once it becomes a nuisance
There was a problem hiding this comment.
Yes - and there is already some implementations that do not follow this, e.g. pkcs11
There was a problem hiding this comment.
Yes, we need to clarify this. Currently you cannot access the pkcs11 plugin via the resource uri, which is essentially a bug imo. I am open to allowing arbitrary length requests, like we do with rv uris, but that is not how the resource uri works today.
| A Resource URI must comply with the following format: | ||
|
|
||
| ```plaintext | ||
| kbs+<plugin>://<kbs_host>:<kbs_port>/<repository>/<type>/<tag> |
There was a problem hiding this comment.
The asymmetry here is that with kbs+<plugin>:// callers need to know the resource is provided by <plugin> but with the resource plugin, they have no way to say the resources comes from vault.
How the plugin URIs can be supported with api-server-rest queries?
There was a problem hiding this comment.
vault is not a plugin level conception, but a resource plugin level. Thus it's expected that the caller does not know that.
for ASR, currently is 127.0.0.1:8006/cdh/resource/.... we could generize it as 127.0.0.1:8006/cdh/<plugin-in-name>/... for kbs+<plugin-in-name>:///...
There was a problem hiding this comment.
I would also that the caller doesn't get to request a concrete backing store for their resource is (say, vault or filesystem), this is up to the KBS, no?
I also like this URI scheme 👍
There was a problem hiding this comment.
vault is not a plugin level conception, but a resource plugin level.
yup, my comment was just that the implementation detail is visible to the user and they don't necessarily understand: why kbs+pcks11:// is needed but kbs+vault:// is not.
There was a problem hiding this comment.
the pkcs11 apis are different from the vaults. see https://github.com/confidential-containers/trustee/blob/main/kbs/src/plugins/implementations/pkcs11.rs#L135 for example wrap-key
There was a problem hiding this comment.
yeah, also worth noting that the resource plugin only supports one storage backend anyway.
| A Resource URI must comply with the following format: | ||
|
|
||
| ```plaintext | ||
| kbs+<plugin>://<kbs_host>:<kbs_port>/<repository>/<type>/<tag> |
There was a problem hiding this comment.
I would also that the caller doesn't get to request a concrete backing store for their resource is (say, vault or filesystem), this is up to the KBS, no?
I also like this URI scheme 👍
| ### Repository, Type, and Tag | ||
|
|
||
| Currently resource URIs have three levels of identifiers/scope. | ||
| The terms `repository`, `type`, and `tag` are somewhat arbitrary. | ||
| These identifiers can be used in any way. |
There was a problem hiding this comment.
yeah, I was also thinking whether it is a good idea to require the same path segment for every plugin, for some this hierarchy might just not make any sense. but it keeps implementation complexity at bay for now and that requirement can be relaxed later once it becomes a nuisance
|
|
||
| * `kbs://example.cckbs.org:8081/alice/decryption-key/1` | ||
| * `kbs:///a/b/c` | ||
| * `kbs+pkcs11:///a/b/c` |
There was a problem hiding this comment.
btw the original resource URI could also support query string, like kbs+plugin1://a/b/c?d=e
|
btw, confidential-containers/trustee#1305 needs |
40bba84 to
6ecaccf
Compare
|
Added implementation. PTAL note that I am not touching the ASR in this PR. We will still have a gap there. Currently you can make a request via the ASR like this I don't think we can make that work with plugins so probably we should add a new endpoint like But that is still a bit limited. Open to suggestions here. Probably will address in a follow-up rather than this one. |
A naive question without a deep dive and implementation experience: what's the blocker to use |
Xynnn007
left a comment
There was a problem hiding this comment.
great work for extension! First round review
| Some(plugin_name.to_string()) | ||
| } else { | ||
| return Err("scheme must be kbs or kbs+<plugin>"); | ||
| }; |
There was a problem hiding this comment.
Just a nit
let plugin = match scheme.as_str() {
SCHEME => None,
s if s.starts_with("kbs+") => {
let plugin_name = s.trim_start_matches("kbs+");
if plugin_name.is_empty() {
return Err("scheme kbs+ requires a plugin name, e.g. kbs+pkcs11");
}
Some(plugin_name.to_string())
}
_ => return Err("scheme must be kbs or kbs+<plugin>"),
};| pub r#type: String, | ||
| pub tag: String, | ||
| pub query: Option<String>, | ||
| pub plugin: Option<String>, |
There was a problem hiding this comment.
Maybe we can directly set this as String, and give a default resource if none is given
There was a problem hiding this comment.
That makes it a little tricky to reconstruct the URI in the whole_uri method. wdyt?
| assert_eq!(resource_from_url, resource); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Can we combine them into a single test with rstest? which will look dry
| /// at CDH startup). | ||
| async fn get_kid(kid: &String) -> Result<Vec<u8>> { | ||
| if kid.starts_with("kbs://") { | ||
| if kid.starts_with("kbs://") || kid.starts_with("kbs+") { |
There was a problem hiding this comment.
I once gave the suggestion to make the plugin a String in ResourceUri struct in previous commit. If it's adopted, here we can simply use kid.starts_with("kbs+").
btw, kid.starts_with("kbs+") might not be enough, we might still need to judge ://. so probably a simple .contains() or a regex can be used. wdyt?
There was a problem hiding this comment.
Ok, instead let me import ResourceUri here and try parsing that way.
| source: source.into(), | ||
| })? | ||
| } else if key_uri.starts_with("kbs://") { | ||
| } else if key_uri.starts_with("kbs://") || key_uri.starts_with("kbs+") { |
| let path = if let Some(rest) = keyid.strip_prefix("kbs://") { | ||
| rest | ||
| } else if let Some(pos) = keyid.find("://") { | ||
| let scheme = &keyid[..pos]; | ||
| if scheme.starts_with("kbs+") { | ||
| &keyid[pos + 3..] | ||
| } else { | ||
| keyid | ||
| } | ||
| } else { | ||
| keyid | ||
| }; |
There was a problem hiding this comment.
Maybe we can import ResourceUri and parse, then extract repository, .. etc
There was a problem hiding this comment.
This function has slightly different behavior than the resource Uri parsing. It allows other schemes and even kids without any scheme. So I don't think it gives us much to use the resource uri crate here.
| pub repository: String, | ||
| pub r#type: String, | ||
| pub tag: String, |
There was a problem hiding this comment.
Note that due to #1468 (comment), we could combine these fields into a Vec<String> for any length paths? wdyt?
There was a problem hiding this comment.
I am open to this but I think we should do another PR for handling the variable length stuff.
Risk of collision if we introduce any other CDH endpoints in the future, which we might. |
Allow the plugin to be specifid as part of the scheme. For example, kbs+plugin:///a/b/c Signed-off-by: Tobin Feldman-Fitzthum <tfeldmanfitz@nvidia.com>
|
sounds reasonable. what about to multiplex current resource field to work as the plugin name, s.t. |
|
well. it is also the same issue |
|
so probably cdh/kbs-plugin/xxx or so is the only choice. for legacy, keep legacy I have an impression that we once talked about this, but cannot find the thread. |
|
Yeah, that seems fine, although it doesn't help use with plugins that have query strings or more arguments. I wonder if there is a way to pass the full resource uri. |
Fixup a couple of checks based on the scheme to allow for plugins Signed-off-by: Tobin Feldman-Fitzthum <tfeldmanfitz@nvidia.com>
The keyprovider does some manipulation of the resource uri. Update this so that it will work if the uri specifies a plugin. Signed-off-by: Tobin Feldman-Fitzthum <tfeldmanfitz@nvidia.com>
Allow plugins in the resource uri scheme Signed-off-by: Tobin Feldman-Fitzthum <tfeldmanfitz@nvidia.com>
Update the resource URI doc and add support for plugins.
Specifically, let's allow the scheme to be something like
kbs+plugin://. This seems pretty reasonable and is backwards compatible.If this looks good, I will add some commits to actually implement the change.